chore(charts): productionize sam-mesh chart - #302
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the sam-mesh Helm chart to support Gateway API, removes the bundled Dex in favor of external OIDC issuers, and enhances security contexts and resource limits across components. It also updates the local kind development environment to utilize cloud-provider-kind for LoadBalancer addresses instead of NodePorts. Feedback on the changes includes: avoiding hardcoded ports in the router's external address to support custom service ports; using a portable kubectl base64 decoding template in the local node script; retaining the nodePort configuration for environments where hostPort is restricted; normalizing gateway.consolePath to prevent double slashes; and allowing a custom storageClass for the router's persistent volume claim.
48a1bf3 to
ea0b5bb
Compare
|
CI fixed in #312 |
router.hostPort binds 4501 TCP+UDP on the pod's node and derives the announced multiaddrs from status.hostIP via the downward API, for TCP and QUIC both. The router appends its own PeerID when leasing, so clients get dialable addresses. --allow-loopback is now gated on router.allowLoopback instead of always passed, and the Service drops its TCP-only nodePort block. router.nodeSelector (empty by default) lets the router be pinned away from sam-node workers: with hostPort set the router announces its own node's IP, and a sam-node scheduled onto that same node cannot reach the hostPort in kind.
The hardcoded five-entry list came from the e2e test fixtures and was already stale for every service under development/examples/ except calc-mcp. Expose it as bootstrap.nodeServices, defaulting to ["*"]. Two workarounds it forced are now gone: the sam-admin binding that enumerated per-node kind ServiceAccounts (local-node-sa never existed), and the local allow policies in everything-mcp's node config.
One Gateway and one HTTPRoute behind gateway.enabled: the cloud's 8 exact control-plane paths, a separately-gated dev /admin rule, and the console under gateway.consolePath shaped exactly like the cloud deployment — the bare prefix 302s to <prefix>/ and a URLRewrite filter strips it before the console. URLRewrite is Extended conformance, so the provider must support it. listeners, hostnames, addresses and annotations pass through to the Gateway API objects verbatim, so TLS, static addresses and provider annotations are all expressible instead of hardcoded. gateway.className has no default and is required when enabled: the class is provider-specific, so a wrong default is worse than a clear error.
Dex is an independent component; bundling it coupled the chart to one dev IdP's config shape. The chart now ships no identity provider: controlPlane.oidcIssuer loses its dex default and must be set, and the kind lane deploys its own throwaway Dex from development/kind/dex.yaml instead.
run.sh now fronts the mesh with cloud-provider-kind: the control plane and console share one gateway address, Dex gets its own, and the router is reached at its node IP — no NodePorts, no extraPortMappings. Dex is deployed from dex.yaml in two passes, since its issuer and the console callback URL are the gateway addresses that only exist once the gateways do; the console is restarted after the final control-plane rollout because it reads the issuer from /info once, at startup. The local node enrolls with a bootstrap token over the gateway, the way a real external node joins. The cluster shrinks to two sam-node workers; add labelled workers to kind-config.yaml to host more examples at once. make kind-down delegates to run.sh so cloud-provider-kind and its envoy containers are torn down in dependency order.
router.externalAddrs: node containers resolve sam-router per-network via --add-host, so the relay test's isolated network works only while the announced multiaddr carries that name; a node-IP announcement is unroutable from there. bootstrap.nodeServices: policy.bats' negative path proves the control plane denies an ungranted service, which only works while sam:role:node has a finite list. The chart default is now ["*"], so the lane declares the grants it tests.
The kind guide now matches the gateway lane: LoadBalancer addresses from cloud-provider-kind, the console under /console on the control plane's address, Dex outside the chart, and a bootstrap-token local node. The use-case docs pick up the two-node default cluster.
The unbounded mcp>=1.0.0 pulled mcp 2.0.0, which dropped mcp.server.fastmcp, so the container crashlooped on import. calc-mcp already pins <2.0.0.
runAsNonRoot + RuntimeDefault seccomp at pod level, no privilege escalation and all capabilities dropped per container. The curl init/bootstrap containers pin runAsUser 100 because the image's USER is non-numeric, which runAsNonRoot can't verify. Postgres is left alone: its entrypoint still runs as root and steps down itself.
Each sam image gains image.tag, falling back to global.imageTag.
Pinning a new controlPlane.adminToken or db password on upgrade only updated the Secret; the pods kept the old env values until a manual restart. A checksum/secrets pod annotation forces the rollout. The db statefulset is left out: postgres keeps the password initdb stored in the PVC, so restarting it on a values change fixes nothing.
The Role allowed get/create/delete/patch/update on all secrets in the namespace, including the admin token and db password. The job only server-side-applies the router-token secret: get/patch are now pinned to it via resourceNames, create stays unscoped (admission cannot match a name on create), and delete/update are dropped.
The libp2p peer ID derives from /data/router.key; on emptyDir every reschedule minted a new identity, forcing re-enrollment and leaving the old peer registered until its lease expired. A volumeClaimTemplate keeps it stable, matching .github/k8s/sam-router-template.yaml. volumeClaimTemplates are immutable: existing installs must delete the router StatefulSet before upgrading (the PVC is created fresh either way).
Without a readiness probe the db Service routed to postgres while initdb or crash recovery was still running. PGDATA is deliberately not moved to a subdirectory: existing PVCs are initialized at the volume root and a new PGDATA would shadow their data.
…workload The router only exposed nodeSelector; the other workloads had nothing, so the chart could not target dedicated or tainted node pools.
They were the only containers without resources, which blocks scheduling in namespaces whose ResourceQuota requires them. Hardcoded small values: the containers only poll /info and POST a couple of requests.
One suite per template covering the load-bearing logic: numeric runAsUser pins, bootstrap RBAC scoping, secret checksum rollout, hostPort/externalAddrs address derivation, gateway routing toggles, the router PVC and OIDC mode, scheduling passthroughs and external postgres. Run with make helm-test; the target installs the helm-unittest plugin on first use. Suites that assert the checksum/secrets annotation also load templates/secrets.yaml, or the include cannot resolve.
Runs make helm-lint and make helm-test (which installs the helm-unittest plugin on first use); helm ships on the ubuntu runners. Path-filtered to charts/ so it stays out of unrelated PRs.
The lane reuses the sam-wi-test kind cluster across runs, so upgrading over a release from before the router volumeClaimTemplates change fails with 'updates to statefulset spec ... are forbidden'. On upgrade failure delete the StatefulSets (PVCs survive) and retry. Fresh clusters, as on hosted CI, never take the retry path.
The default RollingUpdate surges a replacement pod before freeing the old pod's hostPort, so on nodes where every schedulable node already runs a replica the rollout deadlocks (FailedScheduling: no free ports). Seen on the reused e2e kind cluster: 2 replicas, 2 workers, hostPort 8080.
Server-side apply falls back to update when the object exists, so patch alone is not always enough; still pinned to the router-token secret via resourceNames.
cfa0b10 to
7b3c172
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the SAM mesh Helm chart and development environment to leverage Gateway API and externalize Dex, while hardening security contexts and adding comprehensive Helm unit tests. Feedback on these changes highlights a potential crash in the kind startup script if the console is disabled, a risk of invalid JSON generation in the bootstrap job if node services are null, and a recommendation to quote path values in the gateway template to prevent YAML parsing issues.
| # The console discovers the issuer from the control plane's /info once, at startup, so | ||
| # restart it now that the control plane serves the Dex issuer. | ||
| echo "== Restarting the console with the final issuer ==" | ||
| kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout restart deployment/sam-mesh-console | ||
| kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout status deployment/sam-mesh-console --timeout=180s |
There was a problem hiding this comment.
If the console is disabled (e.g., via --set console.enabled=false), the sam-mesh-console deployment will not exist. Since set -e is active in this script, attempting to restart or wait for a non-existent deployment will cause the script to crash immediately. Wrap the console restart logic in a check to ensure the deployment exists before attempting to restart it.
| # The console discovers the issuer from the control plane's /info once, at startup, so | |
| # restart it now that the control plane serves the Dex issuer. | |
| echo "== Restarting the console with the final issuer ==" | |
| kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout restart deployment/sam-mesh-console | |
| kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout status deployment/sam-mesh-console --timeout=180s | |
| # The console discovers the issuer from the control plane's /info once, at startup, so | |
| # restart it now that the control plane serves the Dex issuer. | |
| if kubectl --context "${KCTX}" -n "${NAMESPACE}" get deployment/sam-mesh-console >/dev/null 2>&1; then | |
| echo "== Restarting the console with the final issuer ==" | |
| kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout restart deployment/sam-mesh-console | |
| kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout status deployment/sam-mesh-console --timeout=180s | |
| fi |
| - matches: | ||
| - path: | ||
| type: Exact | ||
| value: {{ .Values.gateway.consolePath }} | ||
| filters: | ||
| - type: RequestRedirect | ||
| requestRedirect: | ||
| path: | ||
| type: ReplaceFullPath | ||
| replaceFullPath: {{ .Values.gateway.consolePath }}/ | ||
| statusCode: 302 | ||
| - matches: | ||
| - path: | ||
| type: PathPrefix | ||
| value: {{ .Values.gateway.consolePath }}/ |
There was a problem hiding this comment.
Quoting path values in the Gateway API HTTPRoute is highly recommended to prevent YAML parsing issues if the consolePath contains special characters or is dynamically templated. Additionally, using printf with quote ensures the trailing slash is safely included within the quoted string.
- matches:
- path:
type: Exact
value: {{ .Values.gateway.consolePath | quote }}
filters:
- type: RequestRedirect
requestRedirect:
path:
type: ReplaceFullPath
replaceFullPath: {{ printf "%s/" .Values.gateway.consolePath | quote }}
statusCode: 302
- matches:
- path:
type: PathPrefix
value: {{ printf "%s/" .Values.gateway.consolePath | quote }}| {"name": "sam:role:sambox", "allowed_services": ["*"], "allowed_targets": ["*"]}, | ||
| {"name": "sam:role:router", "allowed_services": ["*"], "allowed_targets": ["*"]}, | ||
| {"name": "sam:role:node", "allowed_services": ["mcp://calculator", "mcp://db-agent", "mcp://http-tool", "mcp://stdio-tool", "system://sam.catalog"], "allowed_targets": ["*"]} | ||
| {"name": "sam:role:node", "allowed_services": {{ toJson .Values.bootstrap.nodeServices }}, "allowed_targets": ["*"]} |
There was a problem hiding this comment.
If .Values.bootstrap.nodeServices is overridden to null or left empty, toJson will output null, which will result in invalid JSON inside the curl payload. Use the default function to fall back to a safe default (like list "*") to ensure the bootstrap job always renders valid JSON.
{"name": "sam:role:node", "allowed_services": {{ toJson (default (list "*") .Values.bootstrap.nodeServices) }}, "allowed_targets": ["*"]}
Adds